Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Case Contact Form Overhaul #6048

Merged
merged 41 commits into from
Oct 21, 2024

Conversation

thejonroberts
Copy link
Contributor

@thejonroberts thejonroberts commented Sep 21, 2024

What github issue is this PR for, if any?

Resolves #XXXX - Is there an issue for this yet?

Please review/merge these PRs first:

They include some of the changes here, but can be merged with no changes to existing behavior. That will make review of this giant PR a little easier.

What changed, and why?

Total overhaul of CaseContact form. Reworked to be in one page. This was accomplished by reducing the wicked wizard steps to one - the details step.

Maintaining the autosave feature was complicated! Before, notes were the only thing that could change during an autosave, but now everything is in the form that gets sent during the autosave update. Therefore, nested_attributes_for contact_topic_answers and additional_expenses required a stimulus NestedForm controller that could create and destroy records -- otherwise, autosave would make new records for every save, or try to delete records that had already been removed in the prior update. This also required creating controllers & routes for those json requests.

How is this tested? (please write tests!) 💖💪

I had to rewrite request and system specs in the process. Request spec was made to be more typical, testing request responses for certain parameters, and not checking page content - I moved those into system specs. I got rid of the system/create spec and moved those tests to new or edit specs as applicable (I'm not sure how create was different from 'new' in a system spec). There were also specs asserting things about case show to that spec rather than the form specs, which I moved over to the case show specs.

Also added request and policy specs for the new controller routes, as mentioned.

Additional Notes

There are a few things from the design requirements that have not been met. I have pending specs for those (which may need to be removed?). I recommend creating issues and handling them separately. They are:

  • Limiting to one answer per ContactTopic, as well as limiting number of topic answers to number of topics
  • Showing ContactTopic description in tooltip. This will require swapping the tooltip out on topic select, and I thought there is a lot of javascript logic already.
  • There may be some other minor things in the design document.
  • I also think it would be good to make an issue for better accessibility. I tried to get the basics out of the way, but one thing that really bothers me is that the form is VERY difficult to tab-navigate through.

Screenshots please :)

localhost_3000_case_contacts_1105_form_details

Feelings gif (optional)

_What gif best describes your feeling working on this issue? https://giphy.com/
alt text

@github-actions github-actions bot added dependencies Pull requests that update a dependency file javascript for use by Github Labeler to mark pull requests that update Javascript code ruby Pull requests that update Ruby code Tests! 🎉💖👏 erb labels Sep 21, 2024
config/routes.rb Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
spec/factories/casa_orgs.rb Outdated Show resolved Hide resolved
@bcastillo32
Copy link
Collaborator

wait WOW @thejonroberts thank you so much for jamming on this. You have made my month 😂

@bcastillo32
Copy link
Collaborator

@thejonroberts thank you for the feedback on the following:

  • Requiring ContactType (validation)
  • Limiting to one answer per ContactTopic, as well as limiting number of topic answers to number of topics
  • Showing ContactTopic description in tooltip. This will require swapping the tooltip out on topic select, and I thought there is a lot of javascript logic already.
  • There may be some other minor things in the design document.
  • I also think it would be good to make an issue for better accessibility. I tried to get the basics out of the way, but one thing that really bothers me is that the form is VERY difficult to tab-navigate through.

I can create some sep tix for these. As fo this PR @elasticspoon do we need an issue to tie this PR to?

@bcastillo32
Copy link
Collaborator

@thejonroberts thank you for the feedback on the following:

  • Requiring ContactType (validation)
  • Limiting to one answer per ContactTopic, as well as limiting number of topic answers to number of topics
  • Showing ContactTopic description in tooltip. This will require swapping the tooltip out on topic select, and I thought there is a lot of javascript logic already.
  • There may be some other minor things in the design document.
  • I also think it would be good to make an issue for better accessibility. I tried to get the basics out of the way, but one thing that really bothers me is that the form is VERY difficult to tab-navigate through.

I can create some sep tix for these. As fo this PR @elasticspoon do we need an issue to tie this PR to?

@thejonroberts just chatted with Mallory (designer) and confirmed that we should be Requiring ContactType - are you able to do a "hot fix" here or would you like a new ticket?

@thejonroberts
Copy link
Contributor Author

@bcastillo32 I can add the ContactType requirement here.

@bcastillo32
Copy link
Collaborator

@bcastillo32 I can add the ContactType requirement here.

Ok great. Thank you so much @thejonroberts !

@thejonroberts thejonroberts force-pushed the contact-form-refactor branch 3 times, most recently from 123adc6 to b2c47b1 Compare September 30, 2024 16:16
Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Honestly, this is really good. I really liked the fallback to additional notes when an org has no contact topics set.

I haven't looked at the code too closely but mainly clicked around a bunch to do some higher level testing:

Blockers

  • delete button for notes does not seem to work?
    • I start a new case contact, add a topic answer, fill it in, reload the page, delete topic answer => is disappears from the page => reload page => its back
    • I see a "you are not authorized' banner at the top of the page (I am admin creating a case contact)
    • logs show ActionController::RoutingError (No route matches [DELETE] "/"):
    • should also probably skip app/controllers/application_controller.rb:39:in set_active_banner' for an api only request
  • creating a contact topic answer:
    • also tries to set_active_banner
    • have this in the logs
ActionController::UnknownFormat (SupervisorsController#index is missing a template for this request format and variant.
request.formats: ["application/json"]
request.variant: []):
  • I think drafts are broken?
    • logged in as a supervisor, picked a case, new case contact
    • filled in some stuff, did not submit
    • went back to page with all contacts for a case and contact was not there
  • I seem to be getting the unauthorized message on random reloads of the contact form

Nice to have (not blocking)

  • add some client side validation to the required fields
  • mark the driving reimbursement fields are required
  • autosave popup would be nice on other fields as well, just so I know when I am good

consolidate to details page
9 pending for notes, additional expenses, autosave
move title to details page
better spec helper, various fixes

delete unused files

additional expenses spec fixes
put contact types partial back
better additional note behavior, comment out autosave, miscellaneous comments, lint
contact topic answer spec and cleanup
todo note for grouped contact types

new spec empty group? inactive type spec
spec fo contact type required

contact type active scope & includes

many small template and spec changes
restore case contact scss

remove form steps
form controller cleanup

remove create_with_answers

form improvements
create/destroy records as needed via js fetch
@elasticspoon
Copy link
Collaborator

I have a long weekend so I'll have time to look over this in the next couple days

@MalloryPriceDesign
Copy link

Hi @thejonroberts, this looks awesome! I'm really excited to see this single-page form coming together. I've just finished reviewing your work for design, so a lot of my feedback will be around aligning the style and UX with our Figma file. I've split this into parts that should be addressed immediately (if possible) and things that could be split into future tickets. Let me know what you think!

To address in this PR:

  • Date auto-populating:
    In the past, some users have complained about submitting forms where data was automatically input for them, such as date. In the Figma file, I was hoping to address this by leaving the date field unpopulated until a user clicks on it. This initial click will still populate it with today's date, as well as open the calendar window.

  • Days since the last contact:
    In the past, we've included an italicized "About 1 week ago" since the last contact note for each Contact Type. Can we add this back? Users found this helpful when filling out the form.
    Screen Shot 2024-10-12 at 3 24 37 PM.

  • Contact Medium options:
    Can you render these options vertically instead of horizontally? This should increase clarity and accessibility.

  • Including One Discussion Note:
    When the page is initialized, we should include one discussion topic and note. Users report wanting to be able to write their notes down quickly, so having a textbox already available should save them a click. Also, can you confirm that these Discussion Topics are being pulled from the organization's configuration? We'll be using these values to map the notes to their corresponding section in the generated court report.

  • Limit to one entry per topic in Notes:
    If it's not too hard, can we limit the number of entries for each topic to one? I'm assuming this should also help our handling of these topics on the backend when generating reports so we don't have to stitch multiple entries per topic together.

  • Reimbursement Visibility
    Can you confirm that the whole reimbursement card is only visible if the user's administrator has enabled reimbursement? Additionally, I cannot see the "Other Expense Amount" and "Other Expense Details" inputs in this section.

Additional feedback

Overall UI Feedback

  1. Consistent horizontal padding - should match Figma
    1. Between "Relevant Case(s)" and "Contact Date"
    2. Between "Hours" and "Minutes"
    3. Between "Discussion Topics" and "Discussion Notes"
    4. Between "Total miles driven" and "Current address"
    5. Padding between the checkboxes and the circle checkboxes and the label should all be the same
  2. Consistent vertical padding - should match Figma
    1. Between each "Discussion Notes" box (vertical padding)
    2. Padding between labels and their associated input box (ie, "Relevant Case(s)" and its input box)
    3. Align padding above "Recrd New Case Contact" with the Figma file
  3. Align the copy to match the Figma file in the text boxes.
    1. For example, update alternative text in the "Relevant Case(s)" input box, from "Select or search" to "Select or search for case(s)"
  4. Color for contact type Group Names should be #5D657B
  5. Align sizes for textboxes, they should all be 20x20, see Figma. Similar note for the circles in the multiple choice questions (ie, "Contact Medium" options)
  6. The options for "Contact Medium" should be vertical
  7. All Labels should be Title Case, ie "Total Miles Driven"
  8. Dummy text to be added where it's missing

Notes Section

  1. The "Discussion Topic" should not be populated by default, with the "Select a discussion topic" alt text showing.
  2. Are these topics hard-coded? They should be pulled from the admin control panel
  3. The form should initialize with one Note visible. The topic should not be selected.
  4. The alt text should be present in the Topic and Notes boxes if nothing is selected or inputted.
  5. Color of the Discussion Topic and Discussion Notes should match the format of headers for other labels, ie "Relevant Case(s)," "Contact Dates"

Reimbursement Section

  1. Are we including the "Other Expense Amount" and "Other Expense Details" input boxes? If not, let's make a ticket for them
  2. Are we maintaining functionality where if an admin has not enabled reimbursement for this org, this whole box isn't viewable to the volunteer
  3. The color of labels should match the format of other labels (similar to the above feedback in the Notes section)
  4. Is it possible to make the height of the "total miles driven" and "current address" input boxes the same?
  5. Update copy to "Mailing Address for Reimbursement Check"

Please let me know if you have any questions about my feedback. Great work so far!

@elasticspoon
Copy link
Collaborator

@elasticspoon , @FireLemons Regarding the autosave questions and my previous comments:

  • I could add a data-action="input->autosave#save" on every single input to fire off an autosave on every change

    • Where to show notifications? Top of every section? No design for these, my want to think on it.
  • OR we could just autosave every X seconds

    • will save a lot of template actions, and make fewer server requests... unless someone walks away from their computer, and it just sends the same update over and over...
  • Maybe a combo, autosave on any input, but stretch out the debounce time to like 30 seconds or a minute?

I think a decent and easy enough solution for now would be to add autosave actions for Additional Expense Description inputs. The expenses may also be difficult to remember/re-enter if you get logged out. I'll add that. I think everything else in the form is easy enough to re-do if you get logged out, and with notes/expenses at the bottom of the form, you will hit an autosave after you have filled them in (typically).

Does that sound like a good approach to y'all?

Yea. I'm good with that as an initial approach and we can see in the future if we need to change that.

Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my stuff is just small comments but the drafts on the case details page (/casa_cases/:case_slug) don't appear, which is actually blocking.

volunteer.mp4

spec/policies/case_contact_policy_spec.rb Outdated Show resolved Hide resolved
spec/policies/case_contact_policy_spec.rb Outdated Show resolved Hide resolved
spec/policies/case_contact_policy_spec.rb Outdated Show resolved Hide resolved
spec/policies/case_contact_policy_spec.rb Outdated Show resolved Hide resolved
spec/policies/case_contact_policy_spec.rb Outdated Show resolved Hide resolved
spec/policies/case_contact_policy_spec.rb Show resolved Hide resolved
spec/policies/contact_topic_answer_policy_spec.rb Outdated Show resolved Hide resolved
spec/system/case_contacts/new_spec.rb Outdated Show resolved Hide resolved
app/javascript/controllers/casa_nested_form_controller.js Outdated Show resolved Hide resolved
spec/requests/case_contacts/form_spec.rb Outdated Show resolved Hide resolved
@thejonroberts
Copy link
Contributor Author

thejonroberts commented Oct 16, 2024

Most of my stuff is just small comments but the drafts on the case details page (/casa_cases/:case_slug) don't appear, which is actually blocking.

@elasticspoon, that cannot be done currently in the app! I tried it out on main branch to confirm.

We don't set CaseContact.casa_case_id until the final submission of the contact. Selected case(s) are in draft_case_id column only. The CasaCase has_many :case_contacts relationship is not there yet, so Case show view is unaware of the drafts. You can only see draft contacts from the CaseContact index (/case_contacts).

@thejonroberts
Copy link
Contributor Author

thejonroberts commented Oct 16, 2024

@MalloryPriceDesign thanks so much for the detailed feedback! I have reworked the form quite a bit and it is much better now! Notes:

To address in this PR:

  • Date auto-populating:

I have made the date input blank on load. Note that there is some weird behavior in Safari browsers - The date input's value is null, but it is displayed as today's date. So user may think it is set, but value is null and will error if form is submitted as is. This could be confusing, so I added basic browser validation to require an input before submit... I think that feedback could probably be improved, depends on the browser implementation.

I also could not get the CSS for input placeholder text color to work consistently for all inputs, I think due to our bootstrap setup. These issues would make good improvement tickets?

  • Days since the last contact:
  • Contact Medium options:
  • Including One Discussion Note:
    When the page is initialized, we should include one discussion topic and note. Users report wanting to be able to write their notes down quickly, so having a textbox already available should save them a click. Also, can you confirm that these Discussion Topics are being pulled from the organization's configuration? We'll be using these values to map the notes to their corresponding section in the generated court report.
  • There is one note shown on load now, unless editing an active contact that had no answers, or organization has no contact topics.
  • These topics are from Organization's contact topics. If there are none available, a disclaimer is shown (as previously), and an 'Additional Notes' field is rendered for contact.notes column.
  • Limit to one entry per topic in Notes:
    If it's not too hard, can we limit the number of entries for each topic to one? I'm assuming this should also help our handling of these topics on the backend when generating reports so we don't have to stitch multiple entries per topic together.
  • I haven't done this yet but will try. I thought of a solution that is likely workable - disabling options that are currently selected... adding and removing completely from list of options is more complicated. Going to attempt it now.
  • I think case contact report will be affected. (uses index_by(&:contact_topic_id): last topic answer will be shown, not others)
  • EDIT: this is now implemented - topics that have been selected are disabled in other selects, 'Add..' button disabled after answers match the number of topics
  • Reimbursement Visibility
    Can you confirm that the whole reimbursement card is only visible if the user's administrator has enabled reimbursement? Additionally, I cannot see the "Other Expense Amount" and "Other Expense Details" inputs in this section.

Yes, the entire Reimbursement section card is not rendered unless organization driving reimbursement is enabled.

  1. Are we including the "Other Expense Amount" and "Other Expense Details" input boxes? If not, let's make a ticket for them
  2. Are we maintaining functionality where if an admin has not enabled reimbursement for this org, this whole box isn't viewable to the volunteer

Expenses are behind another feature flag (also in Organization Edit available to admin), so they are not shown unless Driving Reimbursement & Expense reimbursement are both enabled. Note that this means creator must request driving reimbursement to request Other Expenses... So you can have no reimbursement, driving reimbursement only, or driving & expense reimbursement. But not only Expense reimbursement.

Additional feedback

Overall UI Feedback

Reworked classes and CSS to be much more consistent with design. Pretty close I think, the Figma files were extremely helpful, thank you.

Notes Section

  1. The "Discussion Topic" should not be populated by default, with the "Select a discussion topic" alt text showing.

This is a little more complex than I want to deal with right now. Because of a database constraint on contact_topic_id (not null), we need a topic id when a new contact topic answer (Discussion Note) is added to the form. I think this could be implemented if the database constraint is removed, which would then allow us to require the Topic ID conditionally. Alternatively, could do some (overly complicated imo) javascript manipulations? Regardless, I think it is better left for future work.

EDIT: I am going to try just removing the database constraint... This would actually help with the one entry per topic mentioned above. This is implemented now.

Reimbursement Section
4. Is it possible to make the height of the "total miles driven" and "current address" input boxes the same?

The only way to do this (easily) means that it can only be a one line entry, as opposed to a more traditional address format, is that okay? Will wait to make the change until that is confirmed.

@thejonroberts
Copy link
Contributor Author

@MalloryPriceDesign - ready for re-review, see above, edited some notes I made earlier. (may need to be deployed to the environment you were looking at, I'm not sure how that was setup)

@elasticspoon - ready for re-review, with the caveat that I have not addressed the CasaCase show page draft issue, as I don't think that was a feature prior to this work, unless I'm missing something.

@MalloryPriceDesign
Copy link

MalloryPriceDesign commented Oct 20, 2024

Reimbursement Section
4. Is it possible to make the height of the "total miles driven" and "current address" input boxes the same?

The only way to do this (easily) means that it can only be a one line entry, as opposed to a more traditional address format, is that okay? Will wait to make the change until that is confirmed.

Let's keep this as-is, I think the users will probably expect the traditional address format instead of a one-line format.

@MalloryPriceDesign - ready for re-review, see above, edited some notes I made earlier. (may need to be deployed to the environment you were looking at, I'm not sure how that was setup)

That's great! I used Codespaces to review previously. However, after running git pull and building the app again in my previous Codespace, I got an error. I restarted with a fresh Codespace and am getting the same error:

This page isn’t working
supreme-disco-rj9q4grx54jfppxg-3000.app.github.dev is currently unable to handle this request.
HTTP ERROR 502

I'll take a look again later.

@elasticspoon
Copy link
Collaborator

Most of my stuff is just small comments but the drafts on the case details page (/casa_cases/:case_slug) don't appear, which is actually blocking.

@elasticspoon, that cannot be done currently in the app! I tried it out on main branch to confirm.

We don't set CaseContact.casa_case_id until the final submission of the contact. Selected case(s) are in draft_case_id column only. The CasaCase has_many :case_contacts relationship is not there yet, so Case show view is unaware of the drafts. You can only see draft contacts from the CaseContact index (/case_contacts).

You are right. My flabbers are ghasted. I totally thought that was something that could be done.

Copy link
Collaborator

@elasticspoon elasticspoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thejonroberts This is pretty tremendous work man. You are a saint to doing all this work and being so thorough about it. I don't see any blocking issues. I'm sure something will come up but I'm fine to send this as is.

I'll let @MalloryPriceDesign chime in if she has any further input.

@MalloryPriceDesign
Copy link

Looks amazing! I rebuilt my Codespace container and it's working now!

This looks good! The only change I would suggest is to enable the calendar dropdown to display on the initial click into the date box. Currently, the user has to go all the way to the right calendar icon to click to open the calendar. Is that something that could be easily changed? If not, I don't think this should block merging and can always be addressed in another ticket.

Screen Shot 2024-10-20 at 9 30 22 PM

@thejonroberts
Copy link
Contributor Author

Thank you, Mallory...

This looks good! The only change I would suggest is to enable the calendar dropdown to display on the initial click into the date box. Currently, the user has to go all the way to the right calendar icon to click to open the calendar. Is that something that could be easily changed? If not, I don't think this should block merging and can always be addressed in another ticket.

This is browser dependent -- Safari opens date picker on click for example. I think any changes to browser input behavior should be separate to make sure time can be spent ensuring cross-browser behaviors and accessibility considerations are maintained, a lot of gotchas can pop up!

@compwron
Copy link
Collaborator

wow wow wow!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file erb javascript for use by Github Labeler to mark pull requests that update Javascript code ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants